Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

@radoslawkrzemien/13604 local testing of GitHub actions #21937

Merged
merged 147 commits into from
Sep 13, 2023
Merged

@radoslawkrzemien/13604 local testing of GitHub actions #21937

merged 147 commits into from
Sep 13, 2023

Conversation

radoslawkrzemien
Copy link
Contributor

Details

Testing framework for running local tests of GitHub Actions workflows to identify issues before pushing them into the repository and testing there

Fixed Issues

$ #13604
$ #13604 (comment)

Tests

  1. Run npm run workflow-test
  2. Verify test run results
  • Verify that no errors appear in the JS console

Offline tests

QA Steps

  1. Run npm run workflow-test
  2. Verify test run results
  • Verify that no errors appear in the JS console

PR Author Checklist

  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I added steps for local testing in the Tests section
    • I added steps for the expected offline behavior in the Offline steps section
    • I added steps for Staging and/or Production testing in the QA steps section
    • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
    • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
  • I included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • Android / native
    • Android / Chrome
    • iOS / native
    • iOS / Safari
    • MacOS / Chrome / Safari
    • MacOS / Desktop
  • I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
  • I followed proper code patterns (see Reviewing the code)
    • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
      • If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
    • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
    • I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
    • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
    • I verified the JSDocs style guidelines (in STYLE.md) were followed
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
  • I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
  • I verified that if a function's arguments changed that all usages have also been updated correctly
  • If a new component is created I verified that:
    • A similar component doesn't exist in the codebase
    • All props are defined accurately and each prop has a /** comment above it */
    • The file is named correctly
    • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
    • The only data being stored in the state is data necessary for rendering and nothing else
    • For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
    • Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
    • All JSX used for rendering exists in the render method
    • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
  • If any new file was added I verified that:
    • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
  • I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Screenshots/Videos

Web
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android

@radoslawkrzemien
Add repo to gitignore
5277afe 
Add repo to gitignore

See: Expensify#13604
@radoslawkrzemien
Add new dependencies
2e25354 
Added new packages, most importantly `Mock-github` for creating a
local repository for testing (to have better control over which
files are accessed by the locally run workflows) and `Act-js` for
programatically triggering the workflows with Act (with features
added on top of it, like mocking API calls, mocking steps, etc.)

See: Expensify#13604
@radoslawkrzemien
Add configs
5816c30 
Added `jest` and `ts` config files in a new folder `workflow_tests`
for the testing of GitHub Actions workflows

See: Expensify#13604
@radoslawkrzemien
Add first proper test stub [WiP]
126d210 
Added the stub of the first proper test, with the more-or-less defined structure, but yet without proper step mocks and result assertions

See: Expensify#13604
@radoslawkrzemien
Refactor test configuration [WiP]
a61eaea 
Updated the mocks/test repo files configuration to be more
readable/maintainable. Also added `name` properties to `preDeploy`
workflow steps, since using `name` is the easiest and most stable way
of referencing the steps for mocking

See: Expensify#13604
@radoslawkrzemien
Fix steps mocks [WiP]
abd6d56 
Updated the step mocks to allow for producing outputs that steer the
further workflow execution. Also added run params to the act call

See: Expensify#13604
@radoslawkrzemien
Add output assertions [WiP]
09a7610 
Added the option to assert the output of workflow run and compare it
to a set of expected values

See: Expensify#13604
@radoslawkrzemien
Add tests for failing lint/test jobs [WiP]
a4f31c6 
Added the tests to check what happens when either of `lint` or
`test` jobs fails, namely if `confirmPassingBuild` job gets executed
and all jobs depending on `lint` and `test` being successful do not

See: Expensify#13604
@radoslawkrzemien
Refactor test structure [WiP]
d48add5 
The test file is getting way too big, I will have to split it into
several files, but for now did some refactoring for maintainability

See: Expensify#13604
@radoslawkrzemien
Clean up test file [WiP]
c7b3b21 
Extracted step mocks, assertion helper methods and utils methods to their separate files to clean up the already cluttered test file

See: Expensify#13604
@radoslawkrzemien
Add new tests [WiP]
b427262 
Added tests for new contributor message job

See: Expensify#13604
@radoslawkrzemien
Migrate from .ts to .js [WiP]
1185420 
Changed all typescript code into javascript code and removed ts-related dependencies

See: Expensify#13604
@radoslawkrzemien
Add method for getting mock steps [WiP]
65353d7 
Added a helper method to utils, that helps with creating step mocks - easier than mocking them by hand, allows reusability and reduces the chance of a typo sneaking in

See: Expensify#13604
@radoslawkrzemien
Add method for getting step assertions [WiP]
d89c80c 
Added a helper method to utils, that helps with creating mock step assertions - easier than typing them by hand, allows reusability and reduces the chance of a typo sneaking in

See: Expensify#13604
@radoslawkrzemien
Remove tsconfig [WiP]
0467112 
Forgot to previously remove tsconfig.json

See: Expensify#13604
@radoslawkrzemien
Add tests for chooseDeployActions/skipDeploy/createNewVersion/updateS…
50818e6 
…taging [WiP]

Added the tests checking for which steps get executed based on given set of conditions

See: Expensify#13604
@radoslawkrzemien
Add tests for push to a different branch [WiP]
79c8f64 
Added the test checking if the workflow does not get executed if the code is pushed to a different branch than main

See: Expensify#13604
@radoslawkrzemien
Add test for events different from push [WiP]
3922e7c 
Added the test checking if the workflow does not get executed if the triggering event is different from `push`

See: Expensify#13604
@radoslawkrzemien
Add documentation skeleton [WiP]
1e578c8 
Started working on the tests documentation

See: Expensify#13604
@radoslawkrzemien
Add file structure to documentation [WiP]
8dbd8f8 
Added a new section to documentation with structure of the workflow testing framework

See: Expensify#13604
@radoslawkrzemien
Add utils description [WiP]
29e12e2 
Added a new section to documentation with the description and example usage of utils helper methods

See: Expensify#13604
@radoslawkrzemien
Add breakdown of the test file structure [WiP]
489e902 
Added a new section to documentation with the detail breakdown of the structure of typical test file

See: Expensify#13604
@radoslawkrzemien
Add minor tweaks [WiP]
949f420 
Self-explanatory

See: Expensify#13604
@radoslawkrzemien
Add test for Slack announcement [WiP]
d51407d 
Added the test checking if the failed workflow gets announced on Slack

See: Expensify#13604
@radoslawkrzemien
Add setup section [WiP]
665dbf9 
Added new section to readme, about setting up the environment for running tests

See: Expensify#13604
@radoslawkrzemien
Add deploy tests [WiP]
31c1cf0 
Added tests for the deploy.yml workflow and fixed the setActParams method

See: Expensify#13604
@radoslawkrzemien
Clean up preDeploy.test.js [WiP]
58d0a66 
Self-explanatory

See: Expensify#13604
@radoslawkrzemien
Tweak preDeploy config[WiP]
d9842c0 
Updated event options and local repo pushed branches

See: Expensify#13604
@radoslawkrzemien
Add test for different event
0371c3c 
Added test checking if deploy workflow does not execute on a different event than push

See: Expensify#13604
@radoslawkrzemien
Add first platformDeploy test
67b9540 
Added test first working test for platformDeploy workflow, along with all the mocks and assertions

See: Expensify#13604
@radoslawkrzemien
Update .eslintrc.js
ffcd370 
Added `tests` and `.github` dirs to the overrides removing check for `no-async-await`

See: Expensify#13604
@radoslawkrzemien
Merge remote-tracking branch 'origin/main' into @radoslawkrzemien/136…
26bb430 
…04-local-testing-of-github-actions

# Conflicts:
#	.github/workflows/platformDeploy.yml
#	.github/workflows/verifyPodfile.yml
@radoslawkrzemien
Merge from upstream
a995ca8 
Updated tests after merge

See: Expensify#13604
@radoslawkrzemien
Copy link
Contributor Author

@dangrous Added tests and .github to .eslintrc.js overrides and resolved conflicts

dangrous
dangrous previously approved these changes Aug 30, 2023
View reviewed changes
Copy link
Contributor

@dangrous dangrous left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sweet! We're now in the merge freeze so this will probably have to wait until that's over.... (And so we'll likely need one more commit to fix merge conflicts at the end there.)

But this works for me until then!

Thanks for your patience as this continues on.

@dangrous
Copy link
Contributor

Oops, looks like some lint errors too

@radoslawkrzemien
Run prettier
1a1bb04 
Run prettier

See: Expensify#13604
@radoslawkrzemien
Merge remote-tracking branch 'origin/main' into @radoslawkrzemien/136…
1bb8730 
…04-local-testing-of-github-actions

# Conflicts:
#	.eslintrc.js
dangrous
dangrous previously approved these changes Aug 31, 2023
View reviewed changes
Copy link
Contributor

@dangrous dangrous left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving but will hold on a merge through the freeze (unless you feel we can go ahead @AndrewGable, it does seem pretty self-contained so it would probably be fine?)

@radoslawkrzemien
Copy link
Contributor Author

Hey guys, any updates? Is there anything left for me to do, or just waiting now for the merge?

@dangrous
Copy link
Contributor

dangrous commented Sep 7, 2023

I think we're just waiting for the merge (the freeze should be up at the end of this week) unless @AndrewGable has any final comments!

@dangrous
Copy link
Contributor

okay we're off the freeze, can you merge main and resolve any conflicts please? Thanks!

@radoslawkrzemien
Merge remote-tracking branch 'origin/main' into @radoslawkrzemien/136…
8ecc604 
…04-local-testing-of-github-actions

# Conflicts:
#	.github/workflows/finishReleaseCycle.yml
#	.github/workflows/preDeploy.yml
#	package-lock.json
@radoslawkrzemien
Resolve conflicts
7d6f91f 
Resolved conflicts and updated tests after merging from upstream

See: Expensify#13604
@dangrous
Copy link
Contributor

Hey @radoslawkrzemien can you give a quick summary of the merge conflicts that you updated? It's hard to know exactly what happened since there's so much code haha. Namely:

  • It looks like the Resolve conflicts commit removes most of the StagingDeployCash steps, is that from something merged in, or intentional for this specific PR?
  • This is nitpicky, but I noticed that most of the mock steps created via createMockStep take the form 'Find open StagingDeployCash', 'Finding open StagingDeployCash', (with the ing in the second part of details) - can you make sure all the ones you added take that form?

@AndrewGable let us know if you notice anything else, otherwise I think I'll be good to merge after that!

@radoslawkrzemien
Copy link
Contributor Author

Hey @dangrous
I have merged the changes from upstream repository, which also included some changes to the workflows and because of that the tests needed to be updated slightly in order to reflect these changes and keep passing

The StagingDeployCash steps have been removed in main and this removal was one of the changes merged, hence I removed the corresponding mocks and assertions as they were no longer needed

As for the createMockStep uses, the first value is the name of the step and has to be exactly the same as in the workflow and the second is the message displayed in the logs and returned in the workflow result so it can be set to anything. First I went with the convention to take the step name and modify it into a continuous verb form (as in Find open StagingDeployCash -> Finding open StagingDeployCash), but after working some more on this task I found it a bit too error prone when creating assertions, and since then I started to use the unmodified step value as the message - its the same way in the script for auto-generating mocks/assertions from the workflow (preGenerateTest.js)

LMK if something needs more clarification

@dangrous
Copy link
Contributor

Great that all makes sense to me! I think we can merge!

@dangrous dangrous merged commit ea8dcd0 into Expensify:main Sep 13, 2023
@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify OSBotify mentioned this pull request Sep 14, 2023
Closed
@OSBotify
Copy link
Contributor

🚀 Deployed to staging by https://github.com/dangrous in version: 1.3.70-0 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@kavimuru
Copy link

@dangrous Could you please help us with 1st step in QA Run npm run workflow-test ?

@dangrous
Copy link
Contributor

Hey @kavimuru ! I think this won't actually need QA, it only affects dev, so y'all can ignore. (Unless I'm mistaken, @radoslawkrzemien )

@radoslawkrzemien
Copy link
Contributor Author

You are correct @dangrous , it should only affect dev

@OSBotify
Copy link
Contributor

🚀 Deployed to production by https://github.com/thienlnam in version: 1.3.70-8 🚀

platform result
🤖 android 🤖 failure ❌
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 failure ❌
🕸 web 🕸 success ✅

@dangrous dangrous mentioned this pull request Dec 8, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sobitneupane sobitneupane sobitneupane approved these changes

@dangrous dangrous dangrous approved these changes

@AndrewGable AndrewGable AndrewGable approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@radoslawkrzemien @dangrous @sobitneupane @AndrewGable @mczernek @OSBotify @kavimuru